feat(pypi): support importing uv.lock file#3785
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for uv.lock files in rules_python, introducing a toml2json conversion utility and updating the lock rule and requirement parsing logic. The review identifies critical compatibility issues, specifically the toml2json tool's reliance on Python 3.11's tomllib and missing serialization for date/time objects. Furthermore, the feedback highlights logic errors in how package extras are handled—which could lead to dependency bloat or failed consistency checks—and suggests improvements for platform resolution and path handling in shell scripts.
| """Parse requirements using uv.lock as the primary source.""" | ||
| ret = _parse_uv_lock_json( | ||
| uv_lock_json = uv_lock_json, | ||
| all_platforms = _get_all_platforms(requirements_by_platform) if requirements_by_platform else [], |
There was a problem hiding this comment.
If requirements_by_platform is empty (which is common when using uv.lock as the primary source), all_platforms will be an empty list. This results in all packages having empty target_platforms, which will likely cause issues in downstream rules that expect platform information for wheel selection.
| all_platforms = _get_all_platforms(requirements_by_platform) if requirements_by_platform else [], | |
| all_platforms = _get_all_platforms(requirements_by_platform) if requirements_by_platform else sorted(platforms.keys()), |
| for extra in pkg.get("provides-extras", pkg.get("extras", [])): | ||
| if extra not in entry["extras"]: | ||
| entry["extras"][extra] = None |
There was a problem hiding this comment.
Including all provides-extras in the requirement line for every package is likely incorrect. provides-extras lists all extras a package defines, not necessarily what was resolved or requested. Including all of them will force the installation of all optional dependencies for every package in the lock file, leading to significant dependency bloat. It might be better to omit extras from the requirement line if the lock file already provides the specific version and URL, or only include the extras that were part of the resolution.
| for dep in pkg.get("dependencies", []): | ||
| extra = dep.get("extra") | ||
| if extra and extra not in uv_packages[name]["extras"]: | ||
| uv_packages[name]["extras"][extra] = None |
There was a problem hiding this comment.
The logic for collecting extras in the consistency check appears to be flawed. It iterates over the dependencies of pkg and adds the extra name to uv_packages[name]["extras"], where name is the name of the requiring package. Later, when checking a package item, it looks for extras in uv_packages[item.name]["extras"]. This will fail to find extras that were requested by other packages.
| python_run = getattr(python, "short_path", python) | ||
|
|
||
| # Make python path absolute since uv changes directory via --directory | ||
| python_abs = "$PWD/" + python_path |
There was a problem hiding this comment.
Prepending $PWD/ to python_path assumes that the path is relative to the execution root. If python_path is already an absolute path, this will result in an invalid path. It is safer to check if the path is absolute before prepending.
| python_abs = "$PWD/" + python_path | |
| python_abs = python_path if python_path.startswith("/") else "$PWD/" + python_path |
- tomllib: try/except fallback to tomli for Python <3.11 - json_serializer: add datetime.date and datetime.time support - all_platforms: use sorted(platforms.keys()) fallback - $PWD/ path: check if python_path is absolute - extra_pip_args: pass through to _parse_uv_lock_json - Add uv_lock tests: multiple packages, extra_pip_args, multi-os - Update plan.md with review cycle instructions
|
Addressed all review comments. Please re-review when you get a chance. @gemini-code-assist |
There was a problem hiding this comment.
Code Review
This pull request introduces support for uv.lock files in rules_python, adding a new lock_format attribute to the lock rule and a toml2json utility for parsing. It also implements a consistency check between uv.lock and requirements files. The reviewer identified a potential runtime error in the uv.lock parser due to unsafe dictionary access for optional hash keys and suggested simplifying redundant logic during hash extraction.
|
|
||
| seen = {} | ||
| for wheel in pkg.get("wheels", []): | ||
| sha256 = wheel["hash"].replace("sha256:", "") |
There was a problem hiding this comment.
The hash key is optional for wheels in the uv.lock file format. Using direct dictionary access wheel["hash"] will cause a runtime error if the key is missing. You should use .get() for safe access, similar to how it's used for sdist hashes.
| sha256 = wheel["hash"].replace("sha256:", "") | |
| sha256 = wheel.get("hash", "").replace("sha256:", "") |
- Add python_3_14 toolchain in MODULE.bazel - Always use Python 3.14 interpreter for uv.lock JSON conversion - Remove tomli fallback from toml2json.py (Python 3.14 has tomllib) - Simplify convert_uv_lock_to_json to use fixed interpreter
- Add bazel-in-bazel integration test under tests/integration/uv_lock/ - Test converts uv.lock TOML to JSON using toml2json tool - Remove dead uv_pypi test data (placeholder only) - Restore tomli fallback in toml2json.py for backward compatibility
|
Addressed all review comments and added Python 3.14 toolchain + integration test. Please re-review. @gemini-code-assist |
There was a problem hiding this comment.
Code Review
This pull request introduces support for uv.lock files as a primary metadata source in rules_python. Key additions include a toml2json tool for parsing, a new uv_lock format for the lock rule, and consistency checks within parse_requirements. Reviewer feedback suggests adopting more idiomatic Starlark dictionary access, implementing safer hash prefix stripping, and improving the portability of shell commands by replacing hardcoded environment variables with Bazel-provided paths.
| python_run = getattr(python, "short_path", python) | ||
|
|
||
| # Make python path absolute since uv changes directory via --directory | ||
| python_abs = python_path if python_path.startswith("/") else "$PWD/" + python_path |
There was a problem hiding this comment.
Hardcoding $PWD/ for absolute path construction in a shell command might be problematic on some platforms or environments. Consider using ctx.bin_dir.path or similar Bazel-provided paths if possible, or ensure this is only executed in a bash-compatible environment where PWD is guaranteed to be set correctly.
|
The latest release of toml.bzl makes bazel_lib a dev dependency. It now uses skylib for the |
Remove the old toml2json Python tool and uv_lock.bzl in favor of the pure Starlark toml.bzl decoder. Update tests to pass toml_decode mock, remove is_rules_python_root references, and fix virtual package test expectations. Clean up BUILD.bazel files that referenced deleted targets.
| ), | ||
| logger = logger, | ||
| ), | ||
| uv_lock = pip_attr.uv_lock, |
There was a problem hiding this comment.
Add a test to ensure this is tested.
… lock support Remove the lock_format attribute and detect whether to use uv lock or uv pip compile from the output file extension (.lock = uv lock, else requirements). Add a Windows bat template for uv lock support. The python interpreter is passed through --python flag consistently.
rickeylev
left a comment
There was a problem hiding this comment.
Overall LGTM. Some minor questions and nits. But overall, this fit in rather nicely with the pipstar code you've written, nice!
| The second element is extra_pip_args should be passed to `whl_library`. | ||
| * `is_multiple_versions`: {type}`bool` `True` if multiple versions have been | ||
| specified for this package. | ||
| * `index_url`: {type}`str` The index URL used to download the package. |
There was a problem hiding this comment.
Is there just one possible index url?
There was a problem hiding this comment.
Yes, only one default, others are extra.
| all_platforms[p] = None | ||
| return sorted(all_platforms) | ||
|
|
||
| def _parse_requirements_with_uv_lock( |
There was a problem hiding this comment.
Why does this function have the uv_lock arg as optiona, and fall back to requirements parsing? Its name would indicate it is for parsing uv lock files
| sha256 = sha256, | ||
| url = url, | ||
| filename = filename, | ||
| # NOTE @aignas 2026-05-17: we don't know if it is yanked because we are not |
There was a problem hiding this comment.
Does uv not handle filtering out yanked versions for us?
Though I suppose a version is likely yanked after uv.lock is generated.
What does pypi do if you try to download a yanked version? error or give it to you?
| strip_prefix = "rules_cc-0.1.5", | ||
| urls = ["https://github.com/bazelbuild/rules_cc/releases/download/0.1.5/rules_cc-0.1.5.tar.gz"], | ||
| ) | ||
| http_archive( |
There was a problem hiding this comment.
fwiw, i think its fine to make it bzlmod only. I thought the uv.lock rule was already bzlmod only anyways?
There was a problem hiding this comment.
This is only to make the loads work easier. The parse_requirements is used in WORKSPACE as well.
Require a strict `uv.lock` suffix for uv lock targets to disambiguate from pip-compile outputs, and fix absolute Python interpreter path evaluation during lockfile generation. Additionally, restore helpful structural comments inside requirement parsing logic and register missing TOML Stardoc dependencies.
|
(agent comment) Addressed the second review cycle comments (enforcing |
There was a problem hiding this comment.
Code Review
This pull request implements support for uv.lock files in rules_python, allowing them to be used as the primary source for package metadata and introducing a rule to generate them. The feedback identifies several issues: a potential path resolution failure in lock.bzl when the Python interpreter path is absolute, an undefined repository error caused by exposing virtual packages with no sources in the hub, and a version mismatch for the toml.bzl dependency between MODULE.bazel and py_repositories.bzl.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| python_path = getattr(python, "path", python) | ||
| command = 'export UV_PYTHON_PATH="$(pwd)/{python}" && "$@" --python "$UV_PYTHON_PATH" && cp "{src_dir}/uv.lock" "{output}"'.format( | ||
| python = python_path, | ||
| src_dir = src_dir, | ||
| output = output.path, | ||
| ) |
There was a problem hiding this comment.
When python is configured as an absolute path (e.g., a system interpreter path), python_path will be absolute. Prepending $(pwd)/ to an absolute path results in an invalid path (e.g., /path/to/workspace//usr/bin/python3), which will cause the build action to fail. We should check if python_path is absolute before prepending $(pwd)/.
python_path = getattr(python, "path", python)
is_absolute = python_path.startswith("/") or (len(python_path) > 2 and python_path[1] == ":")
uv_python_path = python_path if is_absolute else "$(pwd)/" + python_path
command = 'export UV_PYTHON_PATH="{python}" && "$@" --python "$UV_PYTHON_PATH" && cp "{src_dir}/uv.lock" "{output}"'.format(
python = uv_python_path,
src_dir = src_dir,
output = output.path,
)
| item = struct( | ||
| name = norm_name, | ||
| is_exposed = True, | ||
| is_multiple_versions = len(versions) > 1, | ||
| # TODO @aignas 2026-05-17: use the default index that is used in parsing the | ||
| # requirements if it is not known in the uv.lock file. We need to get this from the | ||
| # pyproject.toml file uv.tool configuration. | ||
| index_url = "", | ||
| srcs = pkg_srcs, | ||
| ) |
There was a problem hiding this comment.
Setting is_exposed = True for virtual packages (which have no sources, i.e., srcs = []) causes the hub repository to expose them as targets. However, since they have no sources, no corresponding whl_library repository is created for them. This leads to undefined repository errors (e.g., @hub_virtual_pkg not found) when Bazel attempts to resolve or query the hub's targets. We should set is_exposed to len(pkg_srcs) > 0 so that virtual packages without sources are not exposed in the hub.
| item = struct( | |
| name = norm_name, | |
| is_exposed = True, | |
| is_multiple_versions = len(versions) > 1, | |
| # TODO @aignas 2026-05-17: use the default index that is used in parsing the | |
| # requirements if it is not known in the uv.lock file. We need to get this from the | |
| # pyproject.toml file uv.tool configuration. | |
| index_url = "", | |
| srcs = pkg_srcs, | |
| ) | |
| item = struct( | |
| name = norm_name, | |
| is_exposed = len(pkg_srcs) > 0, | |
| is_multiple_versions = len(versions) > 1, | |
| # TODO @aignas 2026-05-17: use the default index that is used in parsing the | |
| # requirements if it is not known in the uv.lock file. We need to get this from the | |
| # pyproject.toml file uv.tool configuration. | |
| index_url = "", | |
| srcs = pkg_srcs, | |
| ) |
| struct( | ||
| name = "virtual_pkg", | ||
| index_url = "", | ||
| is_exposed = True, | ||
| is_multiple_versions = False, | ||
| srcs = [], | ||
| ), |
There was a problem hiding this comment.
Update the unit test expectation to match the corrected is_exposed behavior for virtual packages (which should be False since they have no sources).
| struct( | |
| name = "virtual_pkg", | |
| index_url = "", | |
| is_exposed = True, | |
| is_multiple_versions = False, | |
| srcs = [], | |
| ), | |
| struct( | |
| name = "virtual_pkg", | |
| index_url = "", | |
| is_exposed = False, | |
| is_multiple_versions = False, | |
| srcs = [], | |
| ) |
Add dedicated parsing support for direct artifact URL sources in `uv.lock` files to ensure compatibility with non-registry dependencies. Implemented by adding a dedicated `_add_direct_url_entry` helper and associated unit test coverage in `parse_requirements_tests.bzl`.
Adopt `str.removeprefix()` for safe hash stripping, sync `toml.bzl` to 0.4.1 in `py_repositories.bzl`, add absolute interpreter path checks in `lock.bzl`, format `uv_lock` docstrings with `VERSION_NEXT_FEATURE`, and remove temporary Python 3.14 toolchains.
|
Addressed the remaining unresolved review threads (adopting |
There was a problem hiding this comment.
Code Review
This pull request introduces support for using uv.lock files as the primary source of package metadata in rules_python. It integrates a pure Starlark TOML decoder to parse uv.lock and extract packages, versions, extras, and sources (wheels, sdists, VCS, and direct URLs). Additionally, it updates the lock rule to support generating uv.lock files and adds comprehensive unit tests. The code review identified critical issues where the shell and batch scripts fail when copying uv.lock onto itself, a correctness bug where assigning all platforms to multiple versions of a package causes repository conflicts during hub building, and an issue where git revisions are not appended to VCS URLs, leading to unpinned dependencies.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # in the project directory, then copy it to the expected output path. | ||
| project_dir="$(dirname "$out")" | ||
| "{{args}}" --directory "$project_dir" "$@" | ||
| cp "$project_dir/uv.lock" "$out" |
There was a problem hiding this comment.
When out is configured as the standard uv.lock (e.g., out = "uv.lock"), project_dir/uv.lock and out refer to the exact same file path. On Unix systems, attempting to copy a file onto itself via cp will fail with an error (cp: '...' and '...' are the same file). Since set -e is enabled, this will cause the script to terminate with a failure.
To prevent this, we should only perform the copy if the source and destination paths are different.
| cp "$project_dir/uv.lock" "$out" | |
| if [[ "$project_dir/uv.lock" != "$out" ]]; then | |
| cp "$project_dir/uv.lock" "$out" | |
| fi |
| for %%f in ("%out%") do set "project_dir=%%~dpf" | ||
| set "project_dir=%project_dir:~0,-1%" | ||
| "{{args}}" --directory "%project_dir%" %* | ||
| copy "%project_dir%\uv.lock" "%out%" |
There was a problem hiding this comment.
Similar to the Unix shell script, when out is configured as uv.lock, the copy command on Windows will attempt to copy the file onto itself, which fails with The file cannot be copied onto itself. and returns an exit code of 1.
We should guard the copy operation to only run when the paths are different.
if not "%project_dir%\uv.lock" == "%out%" copy "%project_dir%\uv.lock" "%out%"
| distribution = info["distribution"], | ||
| extra_pip_args = extra_pip_args or [], | ||
| requirement_line = requirement_line, | ||
| target_platforms = list(all_platforms), |
There was a problem hiding this comment.
Setting target_platforms = list(all_platforms) for all package versions in _parse_uv_lock introduces a correctness bug when a package has multiple versions in uv.lock (e.g., due to different Python versions or environment markers).
Because all versions of the package are assigned the exact same target_platforms, they will all generate the same config settings in _add_whl_library. This causes a conflict during the hub building phase, resulting in a build failure: attempting to override an existing repo '...' for config setting '...' with a new repo '...'.
To fix this, we should parse and evaluate the resolution-markers (or package-level markers) for each package version against the configured platforms to determine the correct subset of target_platforms for each version. If no markers are present, we can default to all_platforms.
| url = source["git"] | ||
| _, _, filename = url.rpartition("/") | ||
| entry["src_entries"].append(struct( | ||
| version = version, | ||
| sha256 = "", | ||
| url = url, | ||
| filename = filename, |
There was a problem hiding this comment.
In _add_vcs_entry, the VCS URL is extracted solely from source["git"]. However, in uv.lock, git sources typically specify the exact commit/revision in a separate rev (or commit/tag/branch) field (e.g., source = { git = "...", rev = "deadbeef..." }).
If we do not append the revision to the URL, Bazel/pip will not be able to pin the dependency to the correct commit, leading to non-deterministic builds or checkout failures.
We should check if rev is present in source and construct a standard VCS URL format (e.g., git+https://github.com/org/foo.git@deadbeef) which rules_python expects.
| url = source["git"] | |
| _, _, filename = url.rpartition("/") | |
| entry["src_entries"].append(struct( | |
| version = version, | |
| sha256 = "", | |
| url = url, | |
| filename = filename, | |
| git_url = source["git"] | |
| _, _, filename = git_url.rpartition("/") | |
| url = "git+" + git_url + "@" + source["rev"] if "rev" in source else git_url | |
| entry["src_entries"].append(struct( | |
| version = version, | |
| sha256 = "", | |
| url = url, | |
| filename = filename, |
rickeylev
left a comment
There was a problem hiding this comment.
Fixed a variety of comments
| python_path = getattr(python, "path", python) | ||
| command = 'export UV_PYTHON_PATH="$(pwd)/{python}" && "$@" --python "$UV_PYTHON_PATH" && cp "{src_dir}/uv.lock" "{output}"'.format( | ||
| python = python_path, | ||
| src_dir = src_dir, | ||
| output = output.path, | ||
| ) |
Part of this is vibe coded, but I thought that the approach might have been rigorous
enough to submit a PR.
The strategy was:
uv.lockfile from thelockrule.uv.lockfile together with the requirements fileand verify things are OK.
Extra things that we could do:
uv.lockscenarios and ensure parity withrequirements.txtfiles.PyPIindex to understand if the packages are yanked or not - lockfile does not have that information.
pyproject.tomlfile to get the index values for each package.Summary:
Closes #3557
Work towards #2787